Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: avoid inserting duplicate dylib_path_envvar when calling cargo run recursively #14464

Merged
merged 2 commits into from
Oct 8, 2024

Conversation

linyihai
Copy link
Contributor

@linyihai linyihai commented Aug 28, 2024

What does this PR try to resolve?

If the current program started by cargo run recursively call into cargo run, the second cargo run will insert search_path into dylib_path_envvar again.

Fixes #14194

How should we test and review this PR?

The first commit adds the test to reflect the issue. The first call to cargo run stores the dylib search path env var to a file. Subsequent calls verify that env var remains the same.

The second commit fixes the behavior by checking if env vars in search_path are a prefix of the slice of env vars in dylib_path_envvar.

@rustbot
Copy link
Collaborator

rustbot commented Aug 28, 2024

r? @ehuss

rustbot has assigned @ehuss.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-build-execution Area: anything dealing with executing the compiler S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 28, 2024
Comment on lines 2160 to 2166
if std::env::var("SUB").is_ok() {{
let paths = match std::env::var_os("{}") {{
Some(var) => std::env::split_paths(&var).collect(),
None => Vec::new(),
}};
let first = paths.first().unwrap();
assert!(paths.iter().skip(1).any(|p| p == first ));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we test for the sympton rather than the specific detail? In this case, its an extra rebuild

Comment on lines 326 to 328
// if current program call into `cargo`, we need to avoid inserting `search_path` before `dylib_path` since
// `dylib_path` was already prefixed with `search_path`.
if &dylib_path.first() != &search_path.first() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is only comparing first() sufficient?

@linyihai
Copy link
Contributor Author

This issue seems need more exploration, thanks for your in advance review.

@linyihai linyihai marked this pull request as draft August 29, 2024 07:24
@linyihai linyihai force-pushed the issue-14194 branch 3 times, most recently from b7c1880 to 1166084 Compare September 5, 2024 02:20
@weihanglo weihanglo added the A-environment-variables Area: environment variables label Oct 1, 2024
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason this approach didn't work is pretty much due to #10358. When Cargo computes fingerprint for a Unit of work, it asks env::var which doesn't contain LIBRARY_PATH or PATH because that is set by Cargo. rerun-if-env-changed only captures environment variables set by users via shell like MY_VAR=1 cargo build. Variables added by Cargo only available when consulting the Command struct of the build script execution via Command::get_envs (like what has done in f91dc78).

As a consequence, if you open the fingerprint JSON file of the build script execution (normally at target/debug/.fingerprint/foo-559e30894cda9925/run-build-script-build-script-build.json), you'll see

{
  "rustc": 11851488446658826000,
  // ...
  "local": [
    {
      "RerunIfEnvChanged": {
        "var": "DYLD_FALLBACK_LIBRARY_PATH",
        "val": null
      }
    }
  ],
  "rustflags": [],
  // ...
}

which shows that Cargo failed to capture the initial library search path set by itself.


There is another way to observe this bug: set the LEVEL env var in the test to a higher number like 5. Any subsequent Cargo invocation after the first one should be fresh. Before doing this verification you need to change std::env!("LEVEL") to std::env::var!("LEVEL") because the former will be captured by rustc dep-info, which trigger a rebuild.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before doing this verification you need to change std::env!("LEVEL") to std::env::var!("LEVEL") because the former will be captured by rustc dep-info, which trigger a rebuild.

I didn't expect such a difference.

I decide to to get the "LEVEL" from the command line argument.

@linyihai linyihai force-pushed the issue-14194 branch 3 times, most recently from ed49366 to f5bf972 Compare October 5, 2024 14:56
@linyihai linyihai marked this pull request as ready for review October 6, 2024 09:27
tests/testsuite/build.rs Outdated Show resolved Hide resolved
tests/testsuite/build.rs Outdated Show resolved Hide resolved
tests/testsuite/build.rs Show resolved Hide resolved
@weihanglo weihanglo changed the title fix: avoid inserting search_path into dylib_path_envvar if the program call into cargo inversely. fix: avoid inserting duplicate dylib_path_envvar when calling cargo run recursively Oct 8, 2024
@weihanglo
Copy link
Member

Thanks!

@bors r+

@bors
Copy link
Collaborator

bors commented Oct 8, 2024

📌 Commit 54dbc2b has been approved by weihanglo

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 8, 2024
@bors
Copy link
Collaborator

bors commented Oct 8, 2024

⌛ Testing commit 54dbc2b with merge fbcd9bb...

@bors
Copy link
Collaborator

bors commented Oct 8, 2024

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing fbcd9bb to master...

@bors bors merged commit fbcd9bb into rust-lang:master Oct 8, 2024
22 checks passed
@linyihai linyihai deleted the issue-14194 branch October 8, 2024 13:43
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 9, 2024
Update cargo

8 commits in ad074abe3a18ce8444c06f962ceecfd056acfc73..15fbd2f607d4defc87053b8b76bf5038f2483cf4
2024-10-04 18:18:15 +0000 to 2024-10-08 21:08:11 +0000
- initial version of checksum based freshness (rust-lang/cargo#14137)
- feat: Add custom completer for completing registry name (rust-lang/cargo#14656)
- Document build-plan as being deprecated (rust-lang/cargo#14657)
- fix(complete): Don't complete files for any value (rust-lang/cargo#14653)
- Add more SAT resolver tests (rust-lang/cargo#14614)
- fix: avoid inserting duplicate `dylib_path_envvar` when calling `cargo run` recursively (rust-lang/cargo#14464)
- chore(deps): bump gix-path from 0.10.9 to 0.10.11 (rust-lang/cargo#14489)
- improve error reporting when feature not found in `activated_features` (rust-lang/cargo#14647)

---

This also adds three license exceptions to Cargo.

* arrayref — BSD-2-Clause
* blake3 — CC0-1.0 OR Apache-2.0 OR Apache-2.0 WITH LLVM-exception
* constant_time_eq — CC0-1.0 OR MIT-0 OR Apache-2.0

These exceptions were added to rustc in rust-lang#126930, so should be fine for Cargo as well.
@rustbot rustbot added this to the 1.83.0 milestone Oct 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-build-execution Area: anything dealing with executing the compiler A-environment-variables Area: environment variables S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cargo appends paths to PATH while building without checking if those paths already exist
6 participants